Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(appsync): separating schema from graphql api #9903

Merged
merged 89 commits into from
Aug 26, 2020

Conversation

BryanPan342
Copy link
Contributor

@BryanPan342 BryanPan342 commented Aug 21, 2020

Separating GraphQL Schema from GraphQL Api to simplify GraphQL Api Props. GraphQL Schema is now its own class and employs static functions to construct GraphQL API.

By default, GraphQL Api will be configured to a code-first approach. To override this, use the schema property to specify a method of schema declaration. For example,

const api = appsync.GraphQLApi(stack, 'api', {
  name: 'myApi',
  schema: appsync.Schema.fromAsset(join(__dirname, 'schema.graphl')),
});

BREAKING CHANGES: AppSync GraphQL Schema declared through static functions as opposed to two separate properties

  • appsync: props SchemaDefinition and SchemaDefinitionFile have been condensed down to a singular property schema
  • appsync: no longer directly exposes CfnGraphQLSchema from GraphQLApi.schema

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@BryanPan342
Copy link
Contributor Author

@asterikx I remember us talking about something similar to this but for s3.. i have a draft for s3 but will change it to follow this flow! We have also been talking about using assets recently but we will need to flesh this out more.

@andrestone thoughts on this? I think this is the most I'm willing to commit feature-wise for this PR, but will add more to both the Schema and GraphQLApi classes.

@andrestone
Copy link
Contributor

Great stuff!

I played with the current build a bit and, after this PR, it feels like the only thing that is really missing is a better experience to code the mapping templates. When coding some basic examples I could rarely use the MappingTemplate helper methods, had to fallback to fromString / fromFile all the time.

Next step, I guess?

@BryanPan342
Copy link
Contributor Author

Great stuff!

I played with the current build a bit and, after this PR, it feels like the only thing that is really missing is a better experience to code the mapping templates. When coding some basic examples I could rarely use the MappingTemplate helper methods, had to fallback to fromString / fromFile all the time.

Next step, I guess?

Yeah there are a couple things that I still need to address to round off the code-first approach. Mainly, functions to simplify the creation of queries/mutations/subscriptions, supporting input and enums, and broadening support on directives.

Other than that, I totally agree.. Mapping Templates is definitely the next step. I have been looking into Direct Lambda Resolvers which honestly is so fantastic for AppSync users. But thinking about helping push @duarten's RFC for Mapping Templates.

@andrestone
Copy link
Contributor

andrestone commented Aug 25, 2020

Other than that, I totally agree.. Mapping Templates is definitely the next step. I have been looking into Direct Lambda Resolvers which honestly is so fantastic for AppSync users. But thinking about helping push @duarten's RFC for Mapping Templates.

I actually find that post a bit misleading. Lambda resolvers were always "direct", the only thing that changes is that now there's no need to declare it manually in a resolver (which is a pretty straightforward step, anyway). There are many downsides (and, ofc, upsides) on using Lambda to resolve your fields (cost is most likely the important one) and I guess it should be considered as a last alternative.

The RFC path feels like it's a longer loop. I guess there's a lot of room for improvement on the MappingTemplate static methods.

Let me know if you're planning to work on those, otherwise I might give it a try.

@BryanPan342
Copy link
Contributor Author

Lambda resolvers were always "direct", the only thing that changes is that now there's no need to declare it manually in a resolver (which is pretty straightforward, anyway). There are many downsides (and, ofc, upsides) on using Lambda to resolve your fields (cost is most likely the most important one) and I guess it should be considered as a last alternative.

Mmmm yeah I see what you mean! There are definitely tradeoffs with everything. I think the Direct Lambda Resolver will benefit people who use the Console or CloudFormation Templates more than CDK users. Mainly because as an IaC service, CDK can over functions to make creating Mapping Templates more fluid 🙃

The RFC path feels like it's a longer loop. I guess there's a lot of room for improvement on the MappingTemplate static methods.

What functions were you thinking of making? I think one of the hardest parts of the MappingTemplate implementation right now is that there is no central focus/model. It was mostly created through community contribution which is great for some use cases, but I'm sure everyone would love it to be more comprehensive!

I'm willing to riff on some static functions to make Mapping Templates a lot more usable though while we wait on @duarten's RFC! If you want we can create a tracking issue for this and start creating artifacts so other community members can contribute to the discussion as well :)

@andrestone
Copy link
Contributor

andrestone commented Aug 25, 2020

Mmmm yeah I see what you mean! There are definitely tradeoffs with everything. I think the Direct Lambda Resolver will benefit people who use the Console or CloudFormation Templates more than CDK users. Mainly because as an IaC service, CDK can over functions to make creating Mapping Templates more fluid 🙃

It's a bold can. As of today, writing VTL in the console is the best bad alternative there is :) That's the main reason why I think we should have a minimally pleasant experience in CDK. I mean, it's ok to write plain VTL here and there but if I have to write all of them, even for a relatively simple API, I'd rather do it in the console and then copy/pasta to my CDK / .vtl files (which is a pretty awful thing to do).

I think one of the hardest parts of the MappingTemplate implementation right now is that there is no central focus/model.

I agree and I think some sort of non-disruptive design must be implemented (nothing major I'd say).

I'm willing to riff on some static functions to make Mapping Templates a lot more usable though while we wait on @duarten's RFC! If you want we can create a tracking issue for this and start creating artifacts so other community members can contribute to the discussion as well :)

Not sure if a tracking issue is needed. I think a small refactoring would be enough to support a better design and would allow us to move faster with this.

Any thoughts, @shivlaks and @MrArnoldPalmer?

@MrArnoldPalmer
Copy link
Contributor

@andrestone I'm all for some basic utils to help users in the meantime while we flesh out a more full experience. I would also really appreciate any community feedback on @duarten's proposal/library for the future "big" solution.

I wrote this issue awhile back to track mapping template improvements #8216. We can use that to track changes we want to add.

Copy link
Contributor

@MrArnoldPalmer MrArnoldPalmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

}
const sep = delimiter ?? '';
this.schema.definition = `${this.schema.definition}${sep}${addition}\n`;
public addToSchema(addition: string, delimiter?: string): void {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: I wonder if we need to surface this here, we could probably just allow users to call api.schema.addToSchema if they need the escape hatch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea.. might be misleading to other schema-first users as well...

Ill remove it and the addObjectType and addInterfaceType in the following PR :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually nvm the queue to merge is fat.. ill remove it rn lmao

@mergify
Copy link
Contributor

mergify bot commented Aug 26, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@BryanPan342
Copy link
Contributor Author

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Aug 26, 2020

Command refresh: success

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 4119220
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Aug 26, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 8d71fa1 into aws:master Aug 26, 2020
@BryanPan342 BryanPan342 deleted the schema branch September 8, 2020 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-appsync Related to AWS AppSync contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants